Skip to content

MPT-21554 generate unique e2e currency code avoiding real ISO codes#354

Merged
d3rky merged 1 commit into
mainfrom
bugfix/MPT-21554/e2e-api-client-can-t-create-currencies-tests-400-bad-request
Jun 23, 2026
Merged

MPT-21554 generate unique e2e currency code avoiding real ISO codes#354
d3rky merged 1 commit into
mainfrom
bugfix/MPT-21554/e2e-api-client-can-t-create-currencies-tests-400-bad-request

Conversation

@jentyk

@jentyk jentyk commented Jun 23, 2026

Copy link
Copy Markdown
Member

🤖 AI-generated PR — Please review carefully.

What

Fixes the intermittent 400 Bad Request - Data is not unique failure when the e2e suite creates a currency (tests/e2e/exchange/currencies).

Why

currency_data derived the 3-letter ISO-style code from 3 hex digits mapped onto a 16-letter alphabet (A–P) — only ~4096 possible codes. Those collided with the real ISO 4217 codes already present in the platform (and with leftover e2e currencies), so currencies_service.create(...) failed non-deterministically. The @pytest.mark.flaky marker is only a label (no --reruns configured), so the failing fixture setup was never retried.

How

  • Generate the code from the full 26-letter uppercase alphabet via secrets.choice, regenerating whenever the candidate matches a real ISO 4217 code.
  • Real codes come from the new iso4217 dev dependency (ISO_CURRENCY_CODES), so a generated code can never equal an existing currency.
  • No API-level retry is added; uniqueness against real currencies is guaranteed at generation time.
  • currency_data["code"] stays consistent with the created currency, so test_create_currency's assertion still holds.

Testing

  • ruff format --check, ruff check, flake8, mypy (full deps), uv lock --check — all pass.
  • 2310 unit tests pass.
  • 20k generated codes verified: all 3-letter A–Z, zero collisions with the 178 real ISO codes.
  • e2e tests require live MPT credentials and were not run locally; they run via the scheduled e2e workflow.

Closes MPT-21554

Release Notes

  • Fixed intermittent e2e currency test failures (400 Bad Request - Data is not unique) by improving the random 3-letter currency code generator to avoid collisions
  • Generated currency codes using the full A–Z uppercase alphabet (secrets.choice) instead of a limited hex-derived mapping
  • Added ISO 4217 collision avoidance in the currency fixture by regenerating candidate codes until they don’t match any real ISO 4217 currency code
  • Introduced the iso4217 dev dependency to source the ISO currency code set used for rejection
  • Updated currency fixture teardown to log warnings (sync and async) instead of printing
  • Verified formatting, linting, and type checks pass, and unit tests succeed

@jentyk jentyk requested a review from a team as a code owner June 23, 2026 10:32
@jentyk jentyk requested review from robcsegal and svazquezco June 23, 2026 10:32
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: b5c42b6a-98bd-47c4-b636-c23df4f8e5d2

📥 Commits

Reviewing files that changed from the base of the PR and between 45286ed and c83d294.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • pyproject.toml
  • tests/e2e/exchange/currencies/conftest.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • softwareone-platform/mpt-extension-skills (manual)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/e2e/exchange/currencies/conftest.py
  • pyproject.toml
📜 Recent review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: build

📝 Walkthrough

Walkthrough

Adds iso4217==1.16.* as a dev dependency in pyproject.toml. Updates tests/e2e/exchange/currencies/conftest.py to import secrets and iso4217.Currency, defines module-level constants for code length, alphabet, and known ISO codes, introduces a _random_currency_code() helper that generates non-colliding 3-letter codes for the currency_data fixture, and replaces print statements with logger.warning in fixture teardown error handling.

Changes

ISO-safe currency code generation in e2e tests

Layer / File(s) Summary
Add iso4217 dev dependency
pyproject.toml
Adds iso4217==1.16.* to the dev dependency group with minor formatting adjustments to surrounding entries.
Random ISO-safe currency code helper and fixture update
tests/e2e/exchange/currencies/conftest.py
Adds CURRENCY_CODE_LENGTH, CURRENCY_CODE_ALPHABET, and ISO_CURRENCY_CODES constants plus a _random_currency_code() helper using secrets to produce random 3-letter uppercase codes excluded from ISO 4217, then replaces the deterministic short_uuid-based code derivation in currency_data with this helper. Updates both synchronous and asynchronous fixture teardown to use logger.warning instead of print for error handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • softwareone-platform/mpt-api-python-client#275: Introduces the currency e2e fixture in tests/e2e/exchange/currencies/conftest.py that is modified here to use random ISO-safe currency codes and structured logging instead of print statements.
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Documentation Up To Date ✅ Passed PR changes are test-only and dependency-bump (iso4217 dev dependency added); no documentation impact required per custom check guidelines for test-only changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown

✅ Found Jira issue key in the title: MPT-21554

Generated by 🚫 dangerJS against c83d294

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Around line 1-20: The ISO_CURRENCY_CODES frozenset initialization on line 11
is using the incorrect attribute from the iso4217 Currency enum. Change the
attribute access from .value to .code when iterating over Currency enum members
in the frozenset comprehension, as the iso4217 library exposes the ISO 4217
currency codes through the .code attribute rather than .value. This will prevent
an AttributeError at module import time.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 18024a42-022b-4580-bd03-ba676c3acf74

📥 Commits

Reviewing files that changed from the base of the PR and between 3b336d2 and 5a80c32.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • pyproject.toml
  • tests/e2e/exchange/currencies/conftest.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • softwareone-platform/mpt-extension-skills (manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
**

⚙️ CodeRabbit configuration file

**: # AGENTS.md

Working protocol for any task in this repository:

  1. Identify the task type and select only the local repository files that are relevant to that task.
  2. Read only those relevant local files before making changes.
  3. If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
  4. Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
  5. If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.

Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.

Documentation Reading Order

When applicable, read the repository documentation in this order:

  1. README.md — repository overview, quick start, and documentation map
  2. docs/usage.md — installation, configuration, Python usage examples, and supported Docker-based commands
  3. docs/architecture.md — layered architecture, directory structure, and key abstractions
  4. docs/local-development.md — Docker-only setup and execution model
  5. docs/testing.md — repository-specific testing strategy and command mapping
  6. docs/contributing.md — repository-specific workflow and links to shared standards
  7. docs/documentation.md — repository-specific documentation rules
  8. docs/unit_tests.md — unit test structure and guidance
  9. docs/e2e_tests.md — end-to-end test setup and execution

Then inspect the code paths relevant to the task:

  • mpt_api_client/mpt_client.py — public sync and async client entry points
  • mpt_api_client/http/ — HTTP clients, services, query state, and reusable mixins
  • mpt_api_client/resources/ — domain resource groups such as catalog, commerce, billing, and integrati...

Files:

  • pyproject.toml
  • tests/e2e/exchange/currencies/conftest.py
**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

  • pyproject.toml
  • tests/e2e/exchange/currencies/conftest.py
🧠 Learnings (8)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
📚 Learning: 2026-04-02T09:35:03.825Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 269
File: tests/e2e/helpdesk/chats/links/test_sync_links.py:18-18
Timestamp: 2026-04-02T09:35:03.825Z
Learning: In this repository’s test suite, flake8-aaa/flake8-aaa codes use short two-digit suffixes (e.g., `# noqa: AAA01`), not three-digit variants like `AAA001`. If you see `# noqa: AAA01` in a test file (e.g., when the Act step is performed via a pytest fixture rather than inline code), treat it as valid and intentional—do not flag it as an invalid/no-longer-needed noqa, and do not suggest removing it even if Ruff reports `RUF102`, since `AAA` is configured under `tool.ruff.lint.external` and these noqa directives are expected to be preserved.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
📚 Learning: 2026-02-02T13:05:41.144Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 201
File: tests/unit/resources/accounts/mixins/test_activatable_mixin.py:132-139
Timestamp: 2026-02-02T13:05:41.144Z
Learning: In the mpt-api-python-client repository, tests are configured to use pytest asyncio mode auto, which auto-detects async test functions and runs them without requiring pytest.mark.asyncio. Reviewers should rely on this behavior for all Python test files under tests/, and avoid adding unnecessary asyncio markers in async tests. Ensure test files in tests/ adhere to this convention unless a specific test requires an explicit marker.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
📚 Learning: 2026-04-02T16:26:46.138Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 275
File: tests/e2e/exchange/currencies/conftest.py:43-43
Timestamp: 2026-04-02T16:26:46.138Z
Learning: In this repo’s Python E2E conftest files, line-specific flake8/wemake suppressions like `# noqa: WPS421` placed inline on individual statements (e.g., on specific `print()` calls in teardown blocks) are intentional and acceptable. When reviewing, avoid suggesting converting these to a file-wide ignore (e.g., adding per-file ignores under `[tool.flake8]` in `pyproject.toml`), since that would over-suppress WPS421 for the entire file. Also treat Ruff `RUF102` about an unknown rule code as expected here when suppressing `WPS421` from wemake-python-styleguide.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
📚 Learning: 2026-04-06T09:03:34.466Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 277
File: tests/e2e/extensibility/extensions/conftest.py:37-40
Timestamp: 2026-04-06T09:03:34.466Z
Learning: In this repo’s E2E test conftest teardown fixtures, it is intentional to catch `MPTAPIError` broadly (not just 404) and to print a diagnostic message instead of re-raising. This avoids failing the test suite during teardown when a resource cannot be deleted in some scenarios. Do not recommend narrowing the `except` clause in these teardown blocks (e.g., checking `error.status == 404`), unless the fixture is clearly not intended as an E2E teardown diagnostic.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.

Applied to files:

  • tests/e2e/exchange/currencies/conftest.py
🔇 Additional comments (2)
tests/e2e/exchange/currencies/conftest.py (1)

39-41: LGTM!

pyproject.toml (1)

36-38: 🔒 Security & Privacy

No issues found with iso4217 dependency specification.

The iso4217==1.16.* version constraint is valid and matches actual PyPI releases (including 1.16.20260101 via PEP 440 compatible release semantics). No known security advisories exist for iso4217.

Comment thread tests/e2e/exchange/currencies/conftest.py
@jentyk jentyk force-pushed the bugfix/MPT-21554/e2e-api-client-can-t-create-currencies-tests-400-bad-request branch from 5a80c32 to 5d789de Compare June 23, 2026 10:57
@jentyk jentyk force-pushed the bugfix/MPT-21554/e2e-api-client-can-t-create-currencies-tests-400-bad-request branch from 5d789de to 45286ed Compare June 23, 2026 11:16
…-21554]

The created_currency fixture derived the 3-letter currency code from
3 hex digits mapped onto a 16-letter alphabet (A-P), giving only ~4096
possible codes. Those collided with the real ISO 4217 codes and with
leftover e2e currencies, so currencies_service.create intermittently
failed with "400 Bad Request - Data is not unique".

Generate the code from the full A-Z alphabet and regenerate whenever the
candidate matches a real ISO 4217 code, sourced from the new iso4217 dev
dependency. This removes the deterministic collision against real
currencies and widens the random space, fixing the flaky setup.

Log the generated code from the currency_data fixture at INFO so it is
captured under "Captured log setup" and visible in failed-test output,
making any future uniqueness failure traceable to the exact code used.
The fixture prints are replaced with module-logger calls.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jentyk jentyk force-pushed the bugfix/MPT-21554/e2e-api-client-can-t-create-currencies-tests-400-bad-request branch from 45286ed to c83d294 Compare June 23, 2026 11:20
@sonarqubecloud

Copy link
Copy Markdown

@d3rky d3rky merged commit 14b6b5d into main Jun 23, 2026
6 checks passed
@d3rky d3rky deleted the bugfix/MPT-21554/e2e-api-client-can-t-create-currencies-tests-400-bad-request branch June 23, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants